Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Initialize on-chain StorageVersion for pallets added after genesis #14641

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Jul 25, 2023

Closes paritytech/polkadot-sdk#109

Problem

Quoting from the above issue:

When adding a pallet to chain after genesis we currently don't set the StorageVersion. So, when calling on_chain_storage_version it returns 0 while the pallet is maybe already at storage version 9 when it was added to the chain. This could lead to issues when running migrations.

Solution

  • Add an exists method to StorageVersion, which can be used to check whether the on-chain storage version for a pallet has been initialized
  • Added a before_all method to the OnRuntimeUpgrade trait with a noop default implementation
  • Modify Executive to call before_all for all migrations before running any other hooks
  • Implement before_all in the pallet proc macro impl of OnRuntimeUpgrade to initialize the on-chain version to the current pallet version if it does not exist. If no current pallet version exists, the on-chain version is initialized to 0.

Other changes in this PR

  • Abstracted repeated boilerplate to access the pallet_name in the pallet expand proc macro.

Some discussion about the placement of before_all

It was decided in paritytech/polkadot-sdk#109 to add before_all directly to OnRuntimeUpgrade. However, unless there are use cases outside of initializing the storage version of a pallet maybe it is confusing it make it part of that trait and should be put elsewhere?

@kianenigma suggested creating a superset of OnRuntimeUpgrade called something like OnPalletRuntimeUpgrade and putting before_all there. I like this suggestion because it removes it from view from the average Substrate developer using OnRuntimeUpgrade, but maybe there'll be complications having two different migration traits (like couldn't combine into tuples anymore)? Open to suggestions here, @bkchr curious to hear what you think.

FAQ

Why create a new hook instead of adding this logic to the pallet pre_upgrade?

Executive currently runs COnRuntimeUpgrade (custom migrations) before AllPalletsWithSystem migrations. We need versions to be initialized before the COnRuntimeUpgrade migrations are run, because COnRuntimeUpgrade migrations may use the on-chain version for critical logic. e.g. VersionedRuntimeUpgrade uses it to decide whether or not to execute.

We cannot reorder COnRuntimeUpgrade and AllPalletsWithSystem so AllPalletsWithSystem runs first, because AllPalletsWithSystem have some logic in their post_upgrade hooks to verify that the on-chain version and current pallet version match. A common use case of COnRuntimeUpgrade migrations is to perform a migration which will result in the versions matching, so if they were reordered these post_upgrade checks would fail.

Why init the on-chain version for pallets without a current storage version?

We must init the on-chain version for pallets even if they don't have a defined storage version so if there is a future version bump, the on-chain version is not automatically set to that new version without a proper migration.

e.g. bad scenario:

  1. A pallet with no 'current version' is added to the runtime
  2. Later, the pallet is upgraded with the 'current version' getting set to 1 and a migration is added to Executive Migrations to migrate the storage from 0 to 1
    a. Runtime upgrade occurs
    b. before_all hook initializes the on-chain version to 1
    c. on_runtime_upgrade of the migration executes, and sees the on-chain version is already 1 therefore think storage is already migrated and does not execute the storage migration

Now, on-chain version is 1 but storage is still at version 0.

By always initializing the on-chain version when the pallet is added to the runtime we avoid that scenario.

TODO

  • Tests
  • Return correct weights from pallet implementation of before_all

@liamaharon liamaharon added A3-in_progress Pull request is in progress. No review needed at this stage. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. labels Jul 25, 2023
@liamaharon liamaharon marked this pull request as ready for review July 26, 2023 07:16
@liamaharon liamaharon requested review from a team July 26, 2023 07:16
@liamaharon liamaharon added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 26, 2023

// Pallet with no current storage version should have the on-chain version initialized to 0.
TestExternalities::default().execute_with(|| {
// Example4 current_storage_version is NoStorageVersionSet.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cool if there was some way I could assert this. Any ideas?

Comment on lines +2216 to +2217
// Set the on-chain version to something different to the current version
StorageVersion::new(100).put::<Example>();
Copy link
Contributor Author

@liamaharon liamaharon Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise the new before_all hook will init it to the correct value

@liamaharon liamaharon requested a review from bkchr July 26, 2023 08:14
@liamaharon liamaharon added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 28, 2023
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Development

Successfully merging this pull request may close these issues.

Set StorageVersion for pallets added after genesis
2 participants